chore: Disallow reserve() in clippy to prevent panics#22386
Conversation
github-actions
Bot
added
functions
There was a problem hiding this comment.
@2010YOUY01
Thanks for the cleanup here. Everything looks good overall. I just had a couple of small suggestions around the clippy::disallowed_methods exemption rationale to help future readers understand the tradeoff more clearly.
| block_size = block_size.saturating_mul(2); | ||
| } | ||
| let to_reserve = len.max(block_size as usize); | ||
| #[expect( |
There was a problem hiding this comment.
The rationale on #[expect(clippy::disallowed_methods)] mentions that block sizing prevents capacity overflow, but Vec::reserve can still fail due to allocation failure. If this path is intentionally kept infallible because the surrounding API cannot return a Result, it would help to call that out explicitly in the reason. Otherwise, it may be worth tracking a follow-up for a fallible StringView builder path using try_reserve so allocation failures can be propagated cleanly.
| if self.in_progress.capacity() < required_cap { | ||
| self.flush_in_progress(); | ||
| let to_reserve = (length as usize).max(self.next_block_size() as usize); | ||
| #[expect( |
There was a problem hiding this comment.
Same thought here on the exemption rationale. The bounded block size helps avoid overflow issues, but it does not prevent allocation failure. It would be helpful to clarify that this is an intentional tradeoff for an infallible API path, or possibly reference a future follow-up for a fallible builder approach. That would make it clearer that this is a narrow exception rather than a general safe-use pattern for the lint.
2 participants
Which issue does this PR close?
Rationale for this change
In #22323, we have fixed panic bugs by replacing
Vec::reserve()withVec::try_reserve()This PR enforces a clippy rule to disallow
Vec::reserve()project-wise, to prevent future violations. Also it is easy to ignore this lint with macro, if we can ensure the safety.What changes are included in this PR?
Are these changes tested?
They are not testable right now. To trigger the panic, we would need to construct an Arrow array with more than
i32::MAXelements, but Arrow arrays are currently backed by flat vectors.However, if Arrow supports an encoding such as RLE in the future, it may become possible to represent such large arrays with constant memory. At that point, these code paths would become vulnerable, so I think it is better to fix them now.
Are there any user-facing changes?